Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make resolution cross-env in filesystem plugin and base package #1128

Conversation

SleeplessByte
Copy link
Member

@SleeplessByte SleeplessByte commented Apr 15, 2019

  • Makes sure it's relative to the location root, not site root
  • Makes sure it's resolving paths, indepedent of file system
  • Makes sure it starts with a slash in case there is no directory structure

Description

Changes the resolution in react-static-plugin-source-filesystem to be cross-env compatible.

  • Stop using hardcoded path separators. These are not the same across environments.
  • Use path.relative for relative resolution instead of replace. This normalises paths first, so it works across environments.
  • Cutoff the extension using path.basename and path.extname, which works across environments.
  • Prefix with a slash if necessary (top-level pages)
  • Change the comment to be correct: make relative to location root, not site root.

This works with path.resolve(...) and ./src/pages.

Changes the resolution in react-static to be cross-env compatible.

  • Throw an error if a plugin resolves a windows path (it expects POSIX paths by then)
  • Use join whenever dealing with real paths
  • Add tests for the behaviour above

Additionally, changes the webpack.config.prod.js to use cross-env resolution for the paths that are considered external.

Changes/Tasks

  • Changed code

Motivation and Context

Fixes #1094
Fixes #1122
Fixes #1123

Screenshots (if appropriate):

Types of changes

  • Refactoring/add tests (refactoring or adding test which isn't a fix or add a feature)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I have updated the documentation accordingly
  • I have updated the CHANGELOG with a summary of my changes
  • My changes have tests around them

* Stop using hardcoded path separators. These are not the same across environments.
* Use path.relative for relative resolution instead of replace. This normalizes paths first
* Cutoff the extension using path.basename and path.extname, which works across environments
* Prefix with a slash if necessary
@SleeplessByte
Copy link
Member Author

I've not added tests yet, and not thoroughly tested this in general.

  • This works in my project.
  • Opened the PR as tracking PR and accepting comments / ideas before tests are done.

@SleeplessByte SleeplessByte changed the title Make resolution cross-env react-static-plugin-source-filesystem: Make resolution cross-env Apr 15, 2019
@tannerlinsley
Copy link
Contributor

This looks great to me. Let me know when you're ready to merge. I am!

@SleeplessByte
Copy link
Member Author

@tannerlinsley I just want to add a few tests to cover this in perpetuity.

Currently making sure this all works with nested paths et cetera, adding tests:

  • both types of directory separators
  • using .resolve in location (absolute, using system separators)
  • using .join in location (relative, but correct)
  • using hardcoded / in location (relative, and incorrect)
  • nested paths (both \ and /)

@tannerlinsley
Copy link
Contributor

Awesome!

@SleeplessByte
Copy link
Member Author

SleeplessByte commented Apr 15, 2019

FWIW: I tested out nested paths etc; these all work as intended.

yarn build still fails, probably similar issue and not related to this plugin :) (see #1123)

Failed exporting HTML for URL blog/post/1 (../src/containers/Post):
Cannot convert undefined or null to object

  • Function.keys

  • Routes.js:38 keys
    [site-name]/[react-static]/src/browser/components/Routes.js:38:1
    4

This is covered by git and other svn; it will automatically correct the linebreak style.

https://eslint.org/docs/rules/linebreak-style#using-this-rule-with-version-control-systems

> For example, the default behavior of git on Windows systems is to convert LF linebreaks to CRLF when checking out files, but to store the linebreaks as LF when committing a change

This rule is therefore redundant.
This correctly passes environment variables to the process across environments.
Single quotes won't work across shells. In this case the argument never needs quotes.
@SleeplessByte
Copy link
Member Author

SleeplessByte commented Apr 16, 2019

⚠️ Don't merge yet ⚠️

Does not yet solve the build issue on windows.

The issue lies in _useStaticInfo.useStaticInfo. It returns an empty hash {}.

}
// Instead of using path.sep, we always want to test for all of them. This makes
// the tests consistent and means we can write tests with either separator
const escapedPathSeps = escapeRegExp(`${path.win32.sep}${path.posix.sep}`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Genius

@tannerlinsley
Copy link
Contributor

This is where the static info is passed into the client bundle: https://github.com/nozzle/react-static/blob/master/packages/react-static/src/bootstrapApp.js#L23-L27

The export process requires the bundle (which is just a commonjs webpack bundle) and calls this function with the siteData, which is then pushed throughout the app via react context.

It's a shame we have to do it this way, but React context instances don't survive across bundles.

@tannerlinsley
Copy link
Contributor

The actual context instance lives here: https://github.com/nozzle/react-static/blob/master/packages/react-static/src/browser/context/staticInfoContext.js

If the staticInfo context isn't there, then it probably means it's not being passed to the bundle function in export here: https://github.com/nozzle/react-static/blob/master/packages/react-static/src/static/exportRoute.js#L111

Or, it's not being properly embedded into window.__routeInfo here: https://github.com/nozzle/react-static/blob/master/packages/react-static/src/static/components/BodyWithMeta.js#L8

Any thoughts?

@SleeplessByte
Copy link
Member Author

@tannerlinsley For funsies I've changed sharedHashesByProp online line 38 of Routes.js:

        // Hydrate sharedDataByHash with the embedded routeInfo
-       Object.keys(sharedHashesByProp).forEach(propKey => {
+       Object.keys(sharedHashesByProp || {}).forEach(propKey => {
          sharedDataByHash[sharedHashesByProp[propKey]] = sharedData[propKey]
        })

Just so it would pass this part (I did not put any sharedData in the config, so this is fine. It should have been an empty hash if the resolution went correctly). However, now it's saying my static pages are suspending (coming from source-filesystem).

I'm a bit at a loss.

Currently looking through the files you just linked.

@tannerlinsley
Copy link
Contributor

Let me pull in your PR and try it out.

@SleeplessByte
Copy link
Member Author

SleeplessByte commented Apr 16, 2019

In order to reproduce:

  • pull in the PR
  • go into react-static/packages/react-static
  • yarn build
  • go into react-static/packages/react-static-plugin-source-filesystem
  • yarn build

Create a new project with the basic template

  • edit static.config.js
    • comment out the dynamic blog post data (so you only have the static pages left)
    • maxThreads: 1 (so all routes are rendered after one another and you can log stuff in-between)
  • edit package.json dependencies
    • use a relative path to react-static
    • use a relative path to the react-static-plugin-source-filesystem
  • yarn install
  • yarn build

To update the local package:

  • rm yarn.lock
  • make sure you yarn build again in the packages
  • yarn install (this copies the local -relative- package to the local node_modules)

@SleeplessByte
Copy link
Member Author

SleeplessByte commented Apr 16, 2019

Alright. More information!

I've gone ahead and tried to figure out where exactly it starts losing information. So I logged after the line you linked from exportRoute:

{ template: '../src/pages/404.js',
  sharedHashesByProp: {},
  data: {},
  path: '404',
  sharedData: {},
  siteData: {} }

{ template: '../src/pages/about.js',
  sharedHashesByProp: {},
  data: {},
  path: 'about',
  sharedData: {},
  siteData: {} }

{ template: '../src/pages/blog.js',
  sharedHashesByProp: {},
  data: {},
  path: 'blog',
  sharedData: {},
  siteData: {} }

{ template: '../src/pages/index.js',
  sharedHashesByProp: {},
  data: {},
  path: '/',
  sharedData: {},
  siteData: {} }

At this point it's all good! I'm now moving on to figure out if the context is correctly set and retained.

@tannerlinsley
Copy link
Contributor

Awesome! Yeah, once you know the data pipeline, it's relatively easy to track.

@tannerlinsley
Copy link
Contributor

Again, if that's the bug, then it's a cross-env one like you're thinking.

@SleeplessByte
Copy link
Member Author

You could try searching the generated static bundle static-app.js for the context created in staticInfoContext.js. If it is in fact bundled in there, then it's an externalizing bug.

What exactly am I looking for? I see static-app.js in my artifacts. It contains bundle 47 which is staticInfoContext:

Object.defineProperty(exports, "staticInfoContext", {
  enumerable: true,
  get: function get() {
    return _staticInfoContext["default"];
  }
});
exports.useStaticInfo = void 0;

var _react = __webpack_require__(0);

var _staticInfoContext = _interopRequireDefault(__webpack_require__(47));

var useStaticInfo = function useStaticInfo() {
  return (0, _react.useContext)(_staticInfoContext["default"]);
};

exports.useStaticInfo = useStaticInfo;

/***/ }),
/* 47 */
/***/ (function(module, exports, __webpack_require__) {

"use strict";
ar _interopRequireDefault = __webpack_require__(6);

Object.defineProperty(exports, "__esModule", {
  value: true
});
exports["default"] = void 0;

var _react = _interopRequireDefault(__webpack_require__(0)); // eslint-disable-next-line


var context = _react["default"].createContext({});

if (typeof document !== 'undefined') {
  context = _react["default"].createContext(window.__routeInfo);
}

var _default = context;
exports["default"] = _default;

...Which is the code to create the context, aka the hook.

This code is minified inside vendors~main.hash.js:

function(e, t, n) {
    "use strict";
    var r = n(2);
    Object.defineProperty(t, "__esModule", {
        value: !0
    }), Object.defineProperty(t, "staticInfoContext", {
        enumerable: !0,
        get: function() {
            return i.default
        }
    }), t.useStaticInfo = void 0;
    var o = n(0),
        i = r(n(103));
    t.useStaticInfo = function() {
        return (0, o.useContext)(i.default)
    }
},
function(e, t, n) {
    "use strict";

    function r(e) {
        return (r = "function" == typeof Symbol && "symbol" == typeof Symbol.iterator ? function(e) {
            return typeof e
        } : function(e) {
            return e && "function" == typeof Symbol && e.constructor === Symbol && e !== Symbol.prototype ? "symbol" : typeof e
        })(e)
    }

    function o(e) {
        return (o = "function" == typeof Symbol && "symbol" === r(Symbol.iterator) ? function(e) {
            return r(e)
        } : function(e) {
            return e && "function" == typeof Symbol && e.constructor === Symbol && e !== Symbol.prototype ? "symbol" : r(e)
        })(e)
    }
    n.r(t);
    var i = n(0),
        a = n.n(i),
        u = (n(111), n(12), n(9)),
        l = n.n(u),
        c = n(47),
        s = n.n(c);


...

function(e, t, n) {
      "use strict";
      var r = n(2);
      Object.defineProperty(t, "__esModule", {
          value: !0
      }), t.default = void 0;
      var o = r(n(0)),
          i = o.default.createContext({});
      "undefined" != typeof document && (i = o.default.createContext(window.__routeInfo));
      var a = i;
      t.default = a
  }

@tannerlinsley
Copy link
Contributor

Okay. It's okay that it's bundled inside the vendors.main.js file. That's for the client.

The fact that it's inside the static bundle is very bad though and confirms my hunch. Webpack is bundling the staticInfoContext.js file in the node stage (the one that produces static-app.js).

So now the question is, why would it be externalizing that file in my environment, and not yours?

@tannerlinsley
Copy link
Contributor

@SleeplessByte
Copy link
Member Author

Ah; gimme a minute or three. I'll be back with either a fish on a hook or a single tear.

@SleeplessByte
Copy link
Member Author

🐟 🦈

@SleeplessByte
Copy link
Member Author

@tannerlinsley let's wait for CI; Amended CHANGELOG.md and the description of this PR.

(🎉 🎉 🎉)

There might be more cross-env issues in other plugins, but at least the starter works again. Feel free to ping/tag me whenever you hit one!

@tannerlinsley
Copy link
Contributor

Curiously, are you formatting with the yarn format prettier command?

@SleeplessByte SleeplessByte changed the title react-static-plugin-source-filesystem: Make resolution cross-env Make resolution cross-env in filesystem plugin and base package Apr 16, 2019
@SleeplessByte
Copy link
Member Author

I think it automatically calls that in my IDE

@tannerlinsley
Copy link
Contributor

Looks like the only thing that needs to be updated is the xml snapshots, which are actually updated in master. Going to merge and go from there!

@tannerlinsley tannerlinsley merged commit cb09081 into react-static:master Apr 16, 2019
@tannerlinsley
Copy link
Contributor

For posterity:

This is likely one of the best PR's and Github interactions I have ever had.

@SleeplessByte SleeplessByte deleted the hotfix/cross-platform-plugin-source-filesystem branch April 16, 2019 14:51
@SleeplessByte
Copy link
Member Author

SleeplessByte commented Apr 16, 2019

Thanks for spending the time explaining how all the moving parts work together!

Looks like the only thing that needs to be updated is the xml snapshots, which are actually updated in master. Going to merge and go from there!

Yeah I saw those tests fail -- is that because of a change I made? I didn't touch the XML package so I found it weird. On the other hand; it's of course building a sitemap and I changed paths :P

EDIT: nvm, I see it's failing in that other pr and on travis

@tannerlinsley
Copy link
Contributor

My pleasure!

@tannerlinsley
Copy link
Contributor

I updated the XML code in another PR, but not the snapshots.

@tannerlinsley
Copy link
Contributor

Code is now available in the 7.0.8 release!

🎉🎉🎉

@digitalkaoz
Copy link
Contributor

@SleeplessByte did you test building the site to an absolute path (e.g. /tmp/out)? Its needed for building in Lambda...https://github.com/nozzle/react-static/blob/master/docs/guides/aws-lambda.md

@LukeStanislaus
Copy link

Hi, I'm still getting the #1122 error in the 7.0.9 release. I'm running through cmd.exe on windows:
image
This is from a completely fresh project. I was able to get it working using the WSL, however. I think that the error is related to cross-env issues.

@SleeplessByte
Copy link
Member Author

SleeplessByte commented Apr 17, 2019

Sidenote, in dev I get the following error with the hotloader (cc @tannerlinsley)

Uncaught SyntaxError: The URL 'http:/[http//localhost]:8080' is invalid

Need to look at this weird mangled hot loading webpack-dev-server url'

@digitalkaoz no I did not. Let me know if that currently breaks or not. I'm happy to figure that one out too if it doesnt work!

@LukeStanislaus you're on 7.0.5

Edit: Apparently this was a known issue and has nothing to do with this PR 🗡

@SleeplessByte
Copy link
Member Author

image

Ha. It's correct once and wrong once 😓 😅

@SleeplessByte
Copy link
Member Author

SleeplessByte commented Apr 17, 2019

When I look at the documentation for hot module replacement, it seems the current webpack.dev config file is outdated. I'll open another PR so you can check it out.

Edit: Fixed in #1135

@digitalkaoz
Copy link
Contributor

digitalkaoz commented Apr 17, 2019 via email

@ghost
Copy link

ghost commented Aug 31, 2019

This issue still seems to be happening in August. Using a Windows ten laptop with latest downloads from yarn.
Using React-static 7.1.0
Also getting this peculiar message when I just enter react-static but not sure if its related.:

init().catch(e => {
^

TypeError: Cannot read property 'catch' of undefined
at Object. (C:\Users\msfay\AppData\Local\Yarn\Data\global\node_modules\react-static\bin\react-static:5:7)
at Module._compile (internal/modules/cjs/loader.js:778:30)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)
at Module.load (internal/modules/cjs/loader.js:653:32)
at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
at Function.Module._load (internal/modules/cjs/loader.js:585:3)
at Function.Module.runMain (internal/modules/cjs/loader.js:831:12)
at startup (internal/bootstrap/node.js:283:19)
at bootstrapNodeJSCore (internal/bootstrap/node.js:622:3)

@SleeplessByte
Copy link
Member Author

@FayeB can you open a new issue for that last thing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment